Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Modified %ssl_virtualhost_ip% to %hostname(fqdn)% #461

Merged
merged 1 commit into from
Feb 25, 2019

Conversation

GowthamShanmugam
Copy link
Contributor

tendrl-bug-id: #460

Signed-off-by: GowthamShanmugasundaram [email protected]

@GowthamShanmugam GowthamShanmugam requested review from a team as code owners February 25, 2019 07:15
@GowthamShanmugam
Copy link
Contributor Author

@mbukatov @dahorak @shtripat please review

@@ -22,7 +22,7 @@
#
# IMPORTANT: Be sure to comment out the DocumentRoot, ProxyPass and
# ProxyPassReverse directives above.
#Redirect permanent / https://%ssl_virtualhost_ip%/
#Redirect permanent / https://%hostname(fqdn)%/
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm thinking, if the format is clear enough, that user should replace the whole string... I would maybe chose some form without the brackets. (But consider it just as my opinion.)

@@ -5,7 +5,7 @@
## This configuration file assumes that mod_ssl package is installed on
## RHEL/CentOS hosts and it's configuration has not been modified.

<VirtualHost %ssl_virtualhost_ip%:443>
<VirtualHost %hostname(fqdn)%:443>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the reason to have fqdn here? Based on Apache documentation, the value here might be fqdn, but it is not recommended:

  • A fully qualified domain name for the IP address of the virtual host (not recommended);

Since we added the 00_ prefix to the tendrl-ssl.conf file (so it is processed before the default ssl.conf), whouldn't be sufficient to use just * or _default_ here?

I've tried it quickly and it seems to work as expected, with one small warning in error_log:

[ssl:warn] [pid 25283] AH02292: Init: Name-based SSL virtual hosts only work for clients with TLS server name indication support (RFC 4366)

My understanding is, that it might cause some problem for browsers without SNI support (very old versions of commonly used browsers) if there will be more than one website on the same server (which is not our case).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just now i talked with @mbukatov, He also suggested using the hostname in redirection only, not here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

only for the redirection we need to change ip as hostname

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And what about the usage of _default_ (or *) instead of particular IP here?
I see 2 benefits from that:

  • the documented ssl configuration will be one step shorter (this part will not need any change against the sample configuration).
  • if the Tendrl server will have two or more interfaces, the interface might be available on all of them (unless restricted on some other level).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree but, it is better we support both right? when user don't want to use all IPs then out document won't help much

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think, that using _default_ might be better default and if user have specific requirements, he should consult it with Apache doc... But it's just my opinion...

Copy link
Contributor Author

@GowthamShanmugam GowthamShanmugam Feb 25, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mbukatov @shtripat your thought on this^

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@GowthamShanmugam after quick discussion with @dahorak , I agree with him that using _default_ is better

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dahorak explained clealry, i am also agree with this change

@GowthamShanmugam GowthamShanmugam force-pushed the issues_460 branch 3 times, most recently from bafd795 to 9485bb9 Compare February 25, 2019 11:50
@@ -22,7 +22,7 @@
#
# IMPORTANT: Be sure to comment out the DocumentRoot, ProxyPass and
# ProxyPassReverse directives above.
#Redirect permanent / https://%ssl_virtualhost_ip%/
#Redirect permanent / https://%ssl_virtualhost_fqdn%/
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here i agree with @dahorak, it is better we use a similar kind of name everywhere align the names, So I have used %ssl_virtualhost_fqdn% instead of %hostname(fqdn)%

tendrl-bug-id: Tendrl#460
bugzilla: 1680576

Signed-off-by: GowthamShanmugasundaram <[email protected]>
@GowthamShanmugam
Copy link
Contributor Author

@mbukatov @shtripat @dahorak all changes done

Copy link
Contributor

@dahorak dahorak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Contributor

@mbukatov mbukatov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good.

@GowthamShanmugam
Copy link
Contributor Author

@shtripat we can merge this PR

Copy link
Member

@shtripat shtripat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@shtripat shtripat merged commit b7ce23e into Tendrl:master Feb 25, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants